Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

chore: add on-event behavior to allowed list for navigation behaviors #3

Open
wants to merge 31 commits into
base: master
Choose a base branch
from

Conversation

lizard-boy
Copy link

Deep links would benefit from allowing on-event triggered behaviors

hgray-instawork and others added 30 commits September 28, 2023 17:53
Reduced complexity of determining the url
The original implementation for handling `<navigator>` elements nested
inside `<nav-route>` elements relied on storing Dom elements in a map by
the id of the route which contained them.

This had several problems:
- A cache had to be created
- The items in the cache could become orphaned from their parent doc
  - Mutations to the doc would not be reflected in the cache
- The cache was never cleared
- The id or index of the implementation could change

This new solution uses a context to wrap every `hv-route` which contains
its own document. This allows child routes to retrieve their Dom element
and retrieve any nested navigator direction from the doc.

Improvements are also made to the initial route by creating all routes
into the stack navigator.
… the existing doc (Instawork#618)

As the first step in deep link support, this update supports full
merging of the existing dom with an incoming deep link document. The
structure of the incoming document remains the same as any other
\<navigator\> document structure with the following exceptions:
- For any \<navigator\> node, the addition of an attribute
`merge="true"` will enable merging the navigator children. Omitting the
attribute will cause any \<navigator\> to replace an existing one of the
same name
- Merging will add any child \<nav-route\> nodes which don't exist and
will update those which do
- All `selected` values from the current document are reset to "false"
to allow the incoming document to set a new selected path

In the example below, the deep link provides the following changes:
1. The shifts-route receives a sub-navigator
2. The tabs-navigator receives an additional 'settings' tab
3. The root-navigator receives a new 'account-popup' route
4. The initial route is now
`root-navigator->tabs-route->tabs-navigator->shifts->shifts-route->past-shifts`

Because all incoming data indicated `merge="true"`, all of the changes
were additive.

Original:
```xml
<doc xmlns="https://hyperview.org/hyperview">
  <navigator id="root-navigator" type="stack">
    <nav-route id="tabs-route">
      <navigator id="tabs-navigator" type="tab">
        <nav-route id="live-shifts-route" href="/biz-app-hub" selected="true" />
        <nav-route id="shifts-route" href="/biz-app-shift-group-list" />
        <nav-route id="account-route" href="/biz-app-account" />
      </navigator>
    </nav-route>
  </navigator>
</doc>
```

Deep link:
```xml
<doc xmlns="https://hyperview.org/hyperview">
  <navigator id="root-navigator" type="stack" merge="true">
    <nav-route id="tabs-route">
      <navigator id="tabs-navigator" type="tab" merge="true">
        <nav-route id="shifts-route" href="" selected="true">
          <navigator id="shifts" type="tab" merge="true">
            <nav-route id="upcoming-shifts" href="biz-app-shift-group-list" selected="false" />
            <nav-route id="past-shifts" href="/biz-app-shift-group-list'" selected="true" />
          </navigator>
        </nav-route>
        <nav-route id="settings" href="/biz-app-account" selected="false" />
      </navigator>
    </nav-route>
    <nav-route id="account-popup" href="/biz-app-account" modal="true" />
  </navigator>
</doc>
```

Merged:
```xml
<doc xmlns="https://hyperview.org/hyperview">
  <navigator id="root-navigator" type="stack" merge="true">
    <nav-route id="tabs-route" selected="false">
      <navigator id="tabs-navigator" type="tab" merge="true">
        <nav-route id="live-shifts-route" href="/biz_app/gigs/live_shifts" selected="false" />
        <nav-route id="shifts-route" href="/biz_app/gigs/groups" selected="true">
          <navigator id="shifts" type="tab" merge="true">
            <nav-route id="upcoming-shifts" href="/biz_app/gigs/groups" selected="false" />
            <nav-route id="past-shifts" href="/biz_app/gigs/groups" selected="true" />
          </navigator>
        </nav-route>
        <nav-route id="account-route" href="/biz_app/account" selected="false" />
        <nav-route id="settings" href="/biz_app/account" selected="false" />
      </navigator>
    </nav-route>
    <nav-route id="account-popup" href="/biz_app/account" modal="true" />
  </navigator>
</doc>
```
- Moving the route search into helpers
- Moving the local types into types.ts
…ute (Instawork#623)

Reflect the user's selections in the document

- Use the 'focus' listener to allow each hv-route to set its selected
state within the document
- De-select any sibling routes to ensure the current is the only
selected

Note: I did not use 'blur' to have each route deselect itself as that
would cause routes in the background to lose their state.

In the video, as the user selects between the different tabs, the
various \<nav-route\> children of `tabs-navigator` are shown to reflect
current selection. When the `company` modal is popped up, the `company`
\<nav-route\> shows selected while its sibling `tabs-route` becomes
deselected. Note that the `tabs-navigator` children retain their correct
selection state.

Asana: https://app.asana.com/0/0/1205197138955014/f


https://github.com/Instawork/hyperview/assets/127122858/0687558f-c500-419a-bd69-bbd0843ade72
…vigators (Instawork#624)

Previous implementation used 'dynamic' and 'modal' routes as the only
two children of `stack` navigators. The urls were cached into a context
and retrieved by id.
This update allows each defined `\<nav-route\>` to be added to its
navigator. These known routes can retain their original url and
presentation and reduce the complexity of navigating between them.

One note about the implementation: the 'initialParams' passed into each
route contain their actual url. When we navigate to a route through a
behavior, the new params are merged with the existing. The url had to
explicitly be deleted from the object as passing a url:undefined would
overwrite the initial url value with an 'undefined' value.

Asana: https://app.asana.com/0/0/1205197138955012/f
When an <hv-route> is initially defined to load its own data via url, it
will store its <doc> in state and will pass it down to its children.
When a deep link is processed a provides additional information for the
route, the route must abandon its local state.doc and rely on the
element passed in from its parent.

Initial doc:
``` xmldoc
<doc ...>
   <navigator id="tabs-navigator" type="tab">
      <nav-route id="shifts-route" href="{% url 'biz-app-sub-navigator' %}" selected="true"/>
   </navigator>
</doc>
```

The `<hv-route>` representing `shifts-route` will have loaded its own
`<doc/>` and will have stored it in its state.

Deep link:
``` xmldoc
<doc ...>
   <navigator id="tabs-navigator" type="tab">
      <nav-route id="shifts-route" href="" selected="true">
         <navigator id="shifts" type="tab" merge="true">
            <nav-route id="upcoming-shifts" href="{% url 'biz-app-shift-group-list' %}" selected="true"/>
            <nav-route id="past-shifts" href="{% url 'biz-app-shift-group-list' %}" selected="false"/>
         </navigator>
      </nav-route>
   </navigator>
</doc>
```

The deep link has provided additional context for `shifts-route` by now
providing a sub-navigator `shifts` and the relevant routes.

The `tabs-navigator` remains unchanged; it still has one route
`shifts-route`, which means the `<hv-route>` representing `shifts-route`
is not unmounted. However, that route contains a `<doc>` in state which
is now invalid.

This code change allows the `<hv-route>` to prioritize data received
from its parent over any doc it has in state.

Asana: https://app.asana.com/0/0/1205197143810570/f
Changed approach to use `getDerivedStateFromProps` to drive the
state.doc value
- use the more performant `getFirstChildTag` since all these locations
know their structure
- simplified the usage of the internal component to not require its own
props type
…nstawork#640)

A few code refactors which will help the next step:
1. Created a helper to determine if a route id is one of the generated
'dynamic/modal' routes
2. Centralized the `screenOptions` for stack navigators to reduce
redundant code
3. Created a better `getId` function for getting the id of dynamic and
defined routes
4. Moved the creation of dynamic routes into its own function
5. Re-use `buildScreen` when building dynamic routes to avoid redundant
code
When a modal is opened, there may be additional navigation which happens
within its content. If the modal implements its own stack, those
navigation actions can occur within the view of the modal. They can be
closed without having to back out of all the screens.

In the following videos, the 'company' view is defined as a modal.
Without the stack navigator, the 'edit' action replaces the 'company'
view and the user has to go back before closing the modal. With the
stack navigator, the user can close the modal at any point.

Note: default react-navigation UI is being used in these videos to
simplify the navigation.

| No stack | With stack |
| ---- | ---- |
|
![nostack](https://github.com/Instawork/hyperview/assets/127122858/8fbf265d-2720-4879-812b-7579205291c5)
|
![withstack](https://github.com/Instawork/hyperview/assets/127122858/fadd5271-02f0-412c-9a48-051830ec0957)
|

Asana: https://app.asana.com/0/1204008699308084/1205225573228523/f
Replaced all uses of 'dynamic' as a type of route to 'card' to better
reflect the implementation details.
…work#643)

Remove instances of
`const { someFunction } = this`
and
`const { someProp } = props`
The initial document may contain multiple <nav-route> elements in a
stack. These will be pushed onto the stack immediately. As they are
closed they should be removed from the dom to reflect the current state.

Note: card and modal routes pushed onto the stack by user activity will
not be added to the dom to reflect state.

Asana: https://app.asana.com/0/0/1205225573228534/f

---
Example: closing the 'company' modal.

Before:
```XML
<doc ...>
  <navigator id="root-navigator" type="stack">
    <nav-route id="tabs-route" selected="false">
      <navigator id="tabs-navigator" type="tab">
        <nav-route id="live-shifts-route" href="/biz_app/gigs/live_shifts" selected="false"/>
        <nav-route id="shifts-route" href="/biz_app/sub-navigator" selected="true"/>
        <nav-route id="account-route" href="/biz_app/account" selected="false"/>
      </navigator>
    </nav-route>
    <nav-route id="company" href="/biz_app/account/company" modal="true" selected="true"/>
  </navigator>
</doc>
```

After:
```XML
<doc ...>
  <navigator id="root-navigator" type="stack">
    <nav-route id="tabs-route" selected="false">
      <navigator id="tabs-navigator" type="tab">
        <nav-route id="live-shifts-route" href="/biz_app/gigs/live_shifts" selected="false"/>
        <nav-route id="shifts-route" href="/biz_app/sub-navigator" selected="true"/>
        <nav-route id="account-route" href="/biz_app/account" selected="false"/>
      </navigator>
    </nav-route>
  </navigator>
</doc>
```
)

Making the tab navigator setup more closely resemble the stack navigator
No need to pass separate props object into the components
- Merged the required new functionality from `route-doc` into the newly
added `DocContext`
- Updated the implementation of HvRoute to provide the getter and setter
- Cleaned up usage and naming of the value

---------

Co-authored-by: Florent Bonomo <[email protected]>
…ork#691)

This is a working prototype of deep links with custom navigators.

- For stack navigators, the custom navigator provides an injection of a
custom router which overrides the behavior of initial state and updates
for new routes
  - In the override, all routes are pushed onto the stack
- For tab navigators, the custom navigator calls a 'navigate' whenever
the initial route name prop changes

This functionality requires mobile branch
https://github.com/Instawork/mobile/tree/hardin/deep-link-url-wip

**Tabs**
With the default behavior, the initial state is correct: shifts-route
(middle tab), and second-shifts (right sub tab). However, the deep link
has no effect. The current tab selection remains.

Deep link
```xml
<doc {% namespaces %}>
  <navigator id="root-navigator" type="stack" merge="true">
    <nav-route id="tabs-route">
      <navigator id="tabs-navigator" type="tab" merge="true">
        <nav-route id="shifts-route" href="{% url 'biz-app-sub-navigator' %}" selected="true">
          <navigator id="shifts" type="tab" merge="true">
            <nav-route id="second-shifts" href="{% url 'biz-app-shift-group-list' %}" selected="true"/>
          </navigator>
        </nav-route>
      </navigator>
    </nav-route>
  </navigator>
</doc>
```

| initial state | deep link |
| -- | -- |
|
![initstate](https://github.com/Instawork/hyperview/assets/127122858/9fcb9021-eb6c-40cb-a3f9-ba7759a678d5)
|
![deeplink](https://github.com/Instawork/hyperview/assets/127122858/3bf36ccd-50aa-4bb7-8c9b-2167d8ac2512)
|

With an override navigator, the deep link is able to change the
selection in both the main tab and the sub-tab navigators.
| initial state | deep link |
| -- | -- |
|
![initstate](https://github.com/Instawork/hyperview/assets/127122858/9fcb9021-eb6c-40cb-a3f9-ba7759a678d5)
|
![deeplink](https://github.com/Instawork/hyperview/assets/127122858/9fd88766-cb00-4880-9ff0-849c3cc9ded1)
|

**Stacks**
With the default behavior, the stack navigator pushes only one screen
onto the stack: the one with a naming matching the "initialRouteName"
property value. Deep links provide no additional functionality.

Initial state (index.xml)
```xml
<doc {% namespaces %}>
  <navigator id="root-navigator" type="stack">
    <nav-route id="tabs-route">
      <navigator id="tabs-navigator" type="tab">
        <nav-route id="live-shifts-route" href="{% url 'biz-app-hub' %}"/>
        <nav-route id="shifts-route" href="{% url 'biz-app-sub-navigator' %}" selected="true"/>
        <nav-route id="account-route" href="{% url 'biz-app-account' %}" selected="true"/>
      </navigator>
    </nav-route>
    <nav-route id="company" href="{% url 'biz-app-account-company' %}" modal="true"/>
  </navigator>
</doc>
```

Deep link
```xml
<doc {% namespaces %}>
  <navigator id="root-navigator" type="stack" merge="true">
    <nav-route id="account" href="{% url 'biz-app-account' %}" modal="true"/>
  </navigator>
</doc>
```

Given this initial state, the desired outcome is to have the "company"
modal on top of the tabs.

| initial state | deep link |
| -- | -- |
|
![initstate](https://github.com/Instawork/hyperview/assets/127122858/22b801b8-b3c6-45d4-a91a-ce93dea950c0)
|
![deeplink](https://github.com/Instawork/hyperview/assets/127122858/b30fe8d7-c437-4970-a4bc-9d146107d84b)
|

After overriding the navigator and router, the stack is able to push all
screens onto the stack in the initial state and is able to push screens
onto the stack when a deep link is received.

NOTE: There is currently a bug causing the stack initial state to open
and immediately close the modal screen. It is caused by the overrides of
tabs and stack working against each other. A separate task will be
created to look into fixes.

| initial state | deep link |
| -- | -- |
|
![initstate](https://github.com/Instawork/hyperview/assets/127122858/b95b81f4-68ec-4bad-a728-0c7a15604fb9)
|
![deeplink](https://github.com/Instawork/hyperview/assets/127122858/370e9d77-a8b2-4216-b5a6-63c05bd08e0e)
|


Asana: https://app.asana.com/0/1204008699308084/1205129681586820/f
Fix for issue where a tab navigation is auto closing any stack modals.

Asana: https://app.asana.com/0/1204008699308084/1205572796301526/f
# Conflicts:
#	src/components/hv-list/index.tsx
#	src/components/hv-section-list/index.tsx
#	src/contexts/index.ts
#	src/contexts/navigator-map.tsx
#	src/core/components/hv-navigator/index.tsx
#	src/core/components/hv-navigator/types.ts
#	src/core/components/hv-route/index.tsx
#	src/core/components/hv-route/types.ts
#	src/services/dom/helpers-legacy.ts
#	src/services/navigator/helpers.test.ts
#	src/services/navigator/helpers.ts
#	src/types-legacy.ts
Resolving CI issues found after merging master with TS migration into
integration branch
Moving the behavior trigger code out of `hyper-ref` into a shared
location to allow access by other components

Asana: https://app.asana.com/0/1204008699308084/1205741965789631/f
…root (Instawork#727)

Follow the commits for the individual steps taken
- moved the 'once' handling methods into /services/behaviors
- updated `BehaviorOptions` to properly reflect the state of the data
- created new `RootOnUpdate` and `OnUpdateCallbacks` types
- added `onUpdate` to all props which occur from `hv-root` to
`hv-screen`
- ported the `onUpdate` method and all of its related methods from
`hv-screen` to `hv-root`
- use the callbacks prop to inject any of the instance-specific
callbacks needed for the `onUpdate`
- inject the new `onUpdate` into the props

Asana: https://app.asana.com/0/1204008699308084/1205741965789634/f

---------

Co-authored-by: Florent Bonomo <[email protected]>
Both `hv-screen` and `hv-route` have the potential to have a null `doc`
in state. This can happen from a bad load, or from an override document
being provided by a parent component.
The code used in `onUpdate` and its related methods were not accounting
for the possibility of a null `doc'.

- Updated `ScreenState` to reflect the values of the state as assigned
in `hv-screen`
- Updated callback signature to allow `getDoc` to return null
- Updated `hv-root` implementation of `onUpdate` and related to account
for null doc
- Updated `HvGetRoot` to account for null doc
- Updated behavior implementations which used `HvGetRoot` to account for
null doc

Asana: https://app.asana.com/0/0/1205741965789643/f
…ork#729)

Implement `hv-route` as a provider of `onUpdate` to allow navigators to
process behaviors

Only routes which own a document are providers, any children which do
not own a document will inherit.

Asana: https://app.asana.com/0/1204008699308084/1205741965789637/f

---------

Co-authored-by: Florent Bonomo <[email protected]>
…Instawork#730)

Implementation of `<behavior>` elements as children of `<navigator>`
elements

- Added an optional `<behavior>` child in schema
- Gather and cache all "load" behaviors per navigator
  - warn developer of any behaviors with non matching triggers
- trigger all behaviors on component load
- Refresh and re-trigger behaviors on content update

See individual commits for a few clean up steps performed as part of the
work

Asana: https://app.asana.com/0/1204008699308084/1205741965790735/f
Deep link requests were not properly triggering navigation events like
tab selections. Behaviors continued to work as expected.

Cause:
There was a chunk of code which was calling `useContext` in the
hv-navigator for two contexts. The result of these were not being used
so [the code was
removed](Instawork@f3e25c0).
Without that context, the `useEffect` of the custom navigators doesn't
fire.

Adding it back as part of the hierarchy restores the functionality.

In the videos below, the deep link should cause a selection of the
"Accounts" tab.

| Before | After |
| -- | -- |
|
![before](https://github.com/Instawork/hyperview/assets/127122858/268e58a2-5beb-4dea-b554-eb5328bd6df9)
|
![after](https://github.com/Instawork/hyperview/assets/127122858/25fc969a-77fa-44c5-a766-fa2bf054ade0)
|
Copy link

@greptile-apps greptile-apps bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

PR Summary

This PR adds support for 'on-event' triggered behaviors in navigation, particularly for deep linking. Key changes include:

  • Modified schema/core.xsd to allow elements in and added 'merge' attribute to and 'modal' attribute to .
  • Implemented HvNavigator component in src/core/components/hv-navigator/index.tsx, supporting stack and tab navigation with nested navigators and dynamic routes.
  • Updated src/core/components/hv-root/index.tsx to handle 'DEEP_LINK' and 'DISPATCH_EVENT' actions, with checks for potential event loops.
  • Refactored src/core/components/hv-route/index.tsx to support on-event behaviors and improve deep linking functionality.
  • Added new navigation helpers in src/services/navigator/helpers.ts for handling dynamic routes and improved route selection and merging.

These changes significantly enhance the flexibility and customization of navigation behaviors in Hyperview, particularly for deep linking scenarios.

30 file(s) reviewed, 26 comment(s)
Edit PR Review Bot Settings

src/contexts/navigation.ts Show resolved Hide resolved
Comment on lines +42 to +46
export type StackScreenOptions = {
headerMode: 'float' | 'screen' | undefined;
headerShown: boolean;
title: string | undefined;
};
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

style: Consider using a union type for headerMode instead of including undefined.

Comment on lines +269 to +274
// Check for event loop formation
if (trigger === 'on-event') {
throw new Error(
'trigger="on-event" and action="dispatch-event" cannot be used on the same element',
);
}
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

logic: This check prevents infinite loops, but it might be too restrictive. Consider allowing 'on-event' triggers for dispatch-event actions in certain controlled scenarios

src/core/components/hv-root/index.tsx Show resolved Hide resolved
src/core/components/hv-root/index.tsx Show resolved Hide resolved

export type EventMapBase = {
// eslint-disable-next-line @typescript-eslint/no-explicit-any
data?: any;
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

style: Using any type reduces type safety. Consider defining a more specific type

Comment on lines +45 to +47
if (!node || !node.childNodes) {
return null;
}
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

style: Consider combining these conditions for more concise code:

Suggested change
if (!node || !node.childNodes) {
return null;
}
if (!node?.childNodes) {

Comment on lines 60 to 62
const selectedChild = elements.find(
child => child.getAttribute('selected')?.toLowerCase() === 'true',
child => child.getAttribute(Types.KEY_SELECTED) === 'true',
);
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

style: Use optional chaining for getAttribute to handle potential null elements

Comment on lines +296 to +302
const cleanedParams: TypesLegacy.NavigationRouteParams = { ...routeParams };
if (cleanedParams.url && isUrlFragment(cleanedParams.url)) {
// When a fragment is used, the original url is used for the route
// setting url to undefined will overwrite the value, so the url has to be
// deleted to allow merging the params while retaining the original url
delete cleanedParams.url;
}
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

logic: Deleting the url property might have unintended side effects. Consider setting it to undefined instead

* If an id is found only in the new doc, the node is added to the current
* the 'merge' attribute on a navigator determines if the children are merged or replaced
*/
const mergeNodes = (current: Element, newNodes: NodeListOf<Node>): void => {
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

style: The mergeNodes function is complex and may benefit from being split into smaller, more focused functions

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants